✨ Support Nuget Central Package Management #4369

balteravishay · 2024-10-06T18:59:35Z

What kind of change does this PR introduce?

Support nuget central package management in detecting pinned dependencies

PR title follows the guidelines defined in our pull request documentation

What is the current behavior?

Today, scorecard only supported nuget pinning dependencies with lockfiles. However, nuget provides package-manager-level assurance that when enabling CPM AND declaring specific versions for all direct dependencies the same level of pinning is provided to the end-user.

What is the new behavior (if this is a feature change)?**

Scorecard will check if Directory.*.props file is present and both:
a. ManagePackageVersionsCentrally attribute is set to true
b. all direct dependencies have a specific version declared

if both are true, all unpinned dependencies will be marked as pinned.

Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Fixes #4252

Special notes for your reviewer

This was discussed in a community call with @spencerschrock and it continues the work that was started in PR #4351

Here is a repo that would be found as pinned by this new checks: foesmm/WolvenKit since it has a github workflow with dotnet restore (without lockfile flag) but declares CPM and all dependencies are pinned to specific versions in the properties file

Does this PR introduce a user-facing change?

-->

NONE

Co-authored-by: Liam Moat <[email protected]> Co-authored-by: Ioana A <[email protected]> Co-authored-by: Mélanie Guittet <[email protected]> Signed-off-by: balteravishay <[email protected]>

Signed-off-by: balteravishay <[email protected]>

spencerschrock · 2024-10-14T19:53:12Z

all direct dependencies have a specific version declared

I seem to remember listing specific versions was considered pinned due to some immutable property of the nuget package manager? (Similar to how the Go ecosystem has a checksum database?) Is there a good link for that? I found https://devblogs.microsoft.com/nuget/nuget-package-signing/, but it seems to be a few years old, and didn't know what the default policy of the tool was these days.

JonDouglas · 2024-10-15T13:34:29Z

@jeffkl could you advise on this PR on supporting CPM in this project? 😄

jeffkl · 2024-10-15T15:58:37Z

checks/raw/testdata/Directory.CPMFalse.packages.props

I'm not sure if it matters in this context, but be aware that the official name for the CPM file is Directory.Packages.props and on case sensitive file systems, it won't get imported if the case of the file name is incorrect. All of these files in testdata have a lowercase p. I'm not familiar enough with this codebase to say for sure if it will matter.

jeffkl · 2024-10-15T16:00:29Z

checks/raw/testdata/Directory.Pinned.packages.props

+    <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
+  </PropertyGroup>
+  <ItemGroup>
+    <PackageVersion Include="SomePackage" Version="4.4.5" />


Package versions are not pinned just because a version is declared. NuGet just uses this version specified when it comes across a <PackageReference Include="SomePackage" /> and emits an error if you specified a version in a project.

To truly "pin" a transitive package version that you do not have a direct <PackageReference /> for, you have to enable transitive pinning: https://learn.microsoft.com/nuget/consume-packages/Central-Package-Management#transitive-pinning

Nevermind, I was wrong what "pinning" meant in this context.

We define "pinned" in a reproducibility sense: "a dependency that is explicitly set to a specific hash instead of
allowing a mutable version or range of versions". Which Avishay added support for via nuget --locked-mode, or the RestoreLockedMode property .

The NuGet claim in the PR description is that:

Today, scorecard only supported nuget pinning dependencies with lockfiles. However, nuget provides package-manager-level assurance that when enabling CPM AND declaring specific versions for all direct dependencies the same level of pinning is provided to the end-user.

Which to my limited understanding, is based on the package immutability nuget.org provides. But I'm not sure that fully covers all scenarios mentioned in the lockfile blog post.

@spencerschrock, could you please elaborate which scenarios mentioned in the lockfile blog post you are referring to as missing from this PR:

nuget.config mismatch: is covered with CPM, since CPM suggests that packages are managed centrally and the user gets warnings when using multiple package sources

Intermediate versions and Floating versions - This PR checks for specifying specific versions and will deem floating versions as "unpinned"

Package deletion is granted by nuget

Package content mismatch is granted by Package Immutability/signing

Maybe, indeed, something that is not provided via CPM and direct dependency version check, as proposed by this PR, is transitive dependency pinning (as proposed by @jeffkl ). I can add that to check to the PR if you feel that would make it make all the requirements.

I've been out of the office on personal time, will take a look in the next day or two.

Day 2: Version 4.0.0 gets published. NuGet now restores version 4.0.0 as it is an exact match.

Typically, we see folks publishing higher versions of their packages as time goes by, not lower versions.

It might be helpful to create documentation that explains the different levels/scenarios of "pinned" dependency checks, with clear examples for each scenario. For instance, detailing how an exact version match qualifies as "pinned," a lock file represents "locked," and so forth, to ensure clarity across all cases.

Most of our documentation uses "pinned" to refer to lock files, so we can certainly try to clarify "pinned" vs "locked".

I did some digging into the history of the check, as well as checking with relevant contributors. It started off as Frozen-Deps, which looked for lockfiles., before being renamed Pinned-Dependencies. Most of the check is focused on lockfiles and immutable hashes (OCI or GitHub), but there is some pinning recognized for package managers with immutable versions (Go and Nuget).

In practice, people pin versions that actually exist

Typically, we see folks publishing higher versions of their packages as time goes by, not lower versions.

I recognize this is an edge case we probably don't need to worry about. I was just highlighting one difference of lockfiles and version pinning, as I clarified the semantics of the check.

To truly "pin" a transitive package version that you do not have a direct for, you have to enable transitive pinning

Is this because one of your direct dependencies may declare a range for one of their dependencies?

It might be helpful to create documentation that explains the different levels/scenarios of "pinned" dependency checks, with clear examples for each scenario. For instance, detailing how an exact version match qualifies as "pinned," a lock file represents "locked," and so forth, to ensure clarity across all cases.

Most of our documentation uses "pinned" to refer to lock files, so we can certainly try to clarify "pinned" vs "locked".

I did some digging into the history of the check, as well as checking with relevant contributors. It started off as Frozen-Deps, which looked for lockfiles., before being renamed Pinned-Dependencies. Most of the check is focused on lockfiles and immutable hashes (OCI or GitHub), but there is some pinning recognized for package managers with immutable versions (Go and Nuget).

In practice, people pin versions that actually exist

Typically, we see folks publishing higher versions of their packages as time goes by, not lower versions.

I recognize this is an edge case we probably don't need to worry about. I was just highlighting one difference of lockfiles and version pinning, as I clarified the semantics of the check.

thanks! idk who owns the naming but might be worth calling this out in a future meeting.

should package managers provide basic support for the definition of pinned dependencies I provided earlier and what's in the docs? (i.e. exact version)? or is the expectation to contribute to the "locked" only definition?

lock files aren't that prevalent in NuGet/.NET. this means that the .NET/NuGet ecosystem would score very low:

it seems like this check might benefit from partial points for exact versions and full points for locked/frozen/etc deps?

or perhaps a new check for "pinned" / "locked" package dependencies would be needed to take all this into account to separate from the CI/CD & docker checks?

Normally, the transitive graph is resolved for you based on what the packages you consume have declared. To "pin" a transitive package to a higher version, you just add a direct dependency on it via <PackageReference Include="SomeTransitivePackage" />. With CPM and transitive pinning, NuGet just does this for you during graph resolution without you having to declare the direct dependency.

spencerschrock · 2024-11-07T19:51:18Z

checks/raw/pinned_dependencies.go

+func collectPostProcessNugetCPMDependencies(unpinnedNugetDependencies []*checker.Dependency,
+	postProcessingData *NugetPostProcessData,
+) {
+	packageVersions := postProcessingData.CpmConfig.PackageVersions
+
+	numFixedVersions, unfixedVersions := countFixedVersions(packageVersions)
+	// if all dependencies are fixed to specific versions, pin all dependencies
+	if numFixedVersions == len(packageVersions) {
+		pinAllNugetDependencies(unpinnedNugetDependencies)


Is CPM all-or-nothing for managing (direct) dependencies, or can you manage only a subset centrally? For example, can you pin foo in a Directory.Packages.props and then have a project with packages foo (with no version because CPM) and bar (with a version range)? As written, this PR may give this "pinned" status even though bar can float.

I want to make sure I understand the feature. CPM is just a configuration feature so you don't need to specify versions in more than one place? The reason I'm asking is to see if any of the rules (pinned to specific version) should also apply to all project files?

CPM is all or nothing for an individual project. By enabling it in Directory.Packages.props its on for all projects but individual projects can still opt-out of it. Once enabled, you'll receive an error if any PackageReference declares a version or if any PackageReference has no corresponding PackageVersion item.

balteravishay · 2024-11-20T17:09:53Z

hey @spencerschrock, thanks for your feedback! any other questions about CPM or can we start the review of this PR?

spencerschrock · 2024-11-21T18:22:05Z

hey @spencerschrock, thanks for your feedback! any other questions about CPM or can we start the review of this PR?

Yep my questions are answered (thanks everyone), I'll get started on the review when I get some time.

spencerschrock · 2024-11-25T22:45:50Z

checks/raw/pinned_dependencies.go


+type NugetPostProcessData struct {
+	CsprojConfigs []dotnetCsprojLockedData
+	CpmConfig     properties.CentralPackageManagementConfig


Let's not export this type

type nugetPostProcessData struct {

spencerschrock · 2024-11-25T22:51:37Z

checks/raw/pinned_dependencies.go

+	if unpinnedDependencies := getUnpinnedNugetDependencies(pinningDependenciesData); len(unpinnedDependencies) > 0 {
+		var nugetPostProcessData NugetPostProcessData
+		if err := retrieveNugetCentralPackageManagement(c, &nugetPostProcessData); err != nil {
+			return err
+		}
+		if err := retrieveCsprojConfig(c, &nugetPostProcessData); err != nil {
+			return err
+		}
+		if nugetPostProcessData.CpmConfig.IsCPMEnabled {
+			collectPostProcessNugetCPMDependencies(unpinnedDependencies, &nugetPostProcessData)
+		} else {
+			collectPostProcessNugetCsprojDependencies(unpinnedDependencies, &nugetPostProcessData)
+		}
+	}
+	return nil


optional: in go it's common to return early to reduce nesting: https://danp.net/posts/reducing-go-nesting/

so you could do something like this if you want (but I won't block on it):

unpinnedDependencies := getUnpinnedNugetDependencies(pinningDependenciesData) if len(unpinnedDependencies) == 0 { return nil } // the rest of the function

spencerschrock · 2024-11-25T22:57:17Z

checks/raw/pinned_dependencies.go

+	if err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{
+		Pattern:       "Directory.*.props",


Does Jeff's comment matter here? If you have the pattern broad for the purposes of the test harness, I can help with that.

I'm not sure if it matters in this context, but be aware that the official name for the CPM file is Directory.Packages.props and on case sensitive file systems, it won't get imported if the case of the file name is incorrect.

spencerschrock · 2024-11-25T23:15:16Z

checks/raw/pinned_dependencies.go

+func countFixedVersions(packages []properties.NugetPackage) (int, string) {
+	var unfixedVersions []string


nit: this counts unfixed versions

spencerschrock · 2024-11-25T23:16:07Z

checks/raw/pinned_dependencies.go

+	numFixedVersions, unfixedVersions := countFixedVersions(packageVersions)
+	// if all dependencies are fixed to specific versions, pin all dependencies
+	if numFixedVersions == len(packageVersions) {


the variable is called numFixedVersions but it should really be numUnfixedVersions ? this logic needs fixed

spencerschrock · 2024-11-25T23:24:50Z

internal/dotnet/properties/properties_test.go

+		{"fixed version range", "[1.0]", true},
+		{"version as variable", "$(ComponentDetectionPackageVersion)", true},


can you clarify the version as a variable result? do we know it's fixed?

spencerschrock · 2024-11-25T23:26:55Z

internal/dotnet/properties/properties_test.go

+		{"fixed version", "10.1.1", true},
+		{"fixed beta version", "10.1.1-beta", true},


according to your link in the regex comment, this is equivalent to "Minimum version, inclusive"?

so this conflicts with some of your tests below?

Support Central Package Management

abbfe84

Co-authored-by: Liam Moat <[email protected]> Co-authored-by: Ioana A <[email protected]> Co-authored-by: Mélanie Guittet <[email protected]> Signed-off-by: balteravishay <[email protected]>

balteravishay requested a review from a team as a code owner October 6, 2024 18:59

balteravishay requested review from justaugustus and raghavkaul and removed request for a team October 6, 2024 18:59

balteravishay temporarily deployed to gitlab October 6, 2024 18:59 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test October 6, 2024 19:00 — with GitHub Actions Inactive

Merge branch 'main' into support-nuget-central-pm

20eb987

balteravishay temporarily deployed to gitlab October 6, 2024 19:00 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test October 6, 2024 19:00 — with GitHub Actions Inactive

license

d839200

Signed-off-by: balteravishay <[email protected]>

balteravishay temporarily deployed to gitlab October 7, 2024 08:31 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test October 7, 2024 08:31 — with GitHub Actions Inactive

Merge branch 'main' into support-nuget-central-pm

dcc47a4

balteravishay temporarily deployed to gitlab October 11, 2024 09:45 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test October 11, 2024 09:46 — with GitHub Actions Inactive

balteravishay mentioned this pull request Oct 15, 2024

BUG Pinned Dependency checks for nuget/.Net does not consider implicit restore #4381

Open

jeffkl reviewed Oct 15, 2024

View reviewed changes

Merge branch 'main' into support-nuget-central-pm

04fc62f

balteravishay requested a review from spencerschrock October 30, 2024 17:05

balteravishay temporarily deployed to gitlab October 30, 2024 17:05 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test October 30, 2024 17:05 — with GitHub Actions Inactive

spencerschrock reviewed Nov 7, 2024

View reviewed changes

Merge branch 'main' into support-nuget-central-pm

deb1350

balteravishay temporarily deployed to gitlab November 20, 2024 17:09 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test November 20, 2024 17:09 — with GitHub Actions Inactive

spencerschrock reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Support Nuget Central Package Management #4369

✨ Support Nuget Central Package Management #4369

balteravishay commented Oct 6, 2024

spencerschrock commented Oct 14, 2024

JonDouglas commented Oct 15, 2024

jeffkl Oct 15, 2024

jeffkl Oct 15, 2024

jeffkl Oct 15, 2024

spencerschrock Oct 15, 2024

balteravishay Oct 21, 2024 •

edited

Loading

spencerschrock Nov 4, 2024 •

edited

Loading

aortiz-msft Nov 6, 2024

spencerschrock Nov 7, 2024

spencerschrock Nov 7, 2024

JonDouglas Nov 7, 2024

jeffkl Nov 18, 2024

spencerschrock Nov 7, 2024

jeffkl Nov 18, 2024

balteravishay commented Nov 20, 2024

spencerschrock commented Nov 21, 2024

spencerschrock Nov 25, 2024

spencerschrock Nov 25, 2024

spencerschrock Nov 25, 2024

spencerschrock Nov 25, 2024

spencerschrock Nov 25, 2024

spencerschrock Nov 25, 2024

spencerschrock Nov 25, 2024

		if err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{
		Pattern: "Directory.*.props",

		func countFixedVersions(packages []properties.NugetPackage) (int, string) {
		var unfixedVersions []string

		{"fixed version range", "[1.0]", true},
		{"version as variable", "$(ComponentDetectionPackageVersion)", true},

		{"fixed version", "10.1.1", true},
		{"fixed beta version", "10.1.1-beta", true},

✨ Support Nuget Central Package Management #4369

Are you sure you want to change the base?

✨ Support Nuget Central Package Management #4369

Conversation

balteravishay commented Oct 6, 2024

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior (if this is a feature change)?**

Which issue(s) this PR fixes

Special notes for your reviewer

Does this PR introduce a user-facing change?

spencerschrock commented Oct 14, 2024

JonDouglas commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balteravishay Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

spencerschrock Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balteravishay commented Nov 20, 2024

spencerschrock commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balteravishay Oct 21, 2024 •

edited

Loading

spencerschrock Nov 4, 2024 •

edited

Loading